Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CoreDNS rewrite support for external services. #656

Closed
wants to merge 21 commits into from

Conversation

aviweit
Copy link

@aviweit aviweit commented Jun 18, 2024

This PR deals with updating CoreDNS configuration to rewrite/resolve external names as described in issue #26.

Fixes #26

@aviweit
Copy link
Author

aviweit commented Jun 18, 2024

End to end tests were made with two EKS AWS clusters with three Import CRs:

  1. internal nginx service
  2. external s3 using vpe dns name
  3. external lambda using vpe dns name

these CRs had been added and removed during the tests - to ensure coredns configmap is properly updated.

It was also tested with two local kind clusters ensuring that coredns is properly updated.

Thanks.

Copy link
Collaborator

@elevran elevran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aviweit - thank you for the pull request and adding this feature!

Can you please

  • use uppercase DNS in function names and fields exposed?
  • move all DNS related functions into their own file so changes to existing code files are minimized?
  • Consider if the use case is best served by rewrite, hosts or file CoreDNS plugin?

Thanks!

config/operator/rbac/role.yaml Show resolved Hide resolved
config/crds/clusterlink.net_imports.yaml Outdated Show resolved Hide resolved
pkg/apis/clusterlink.net/v1alpha1/import.go Outdated Show resolved Hide resolved
}

// DeleteImport removes the listening socket of a previously imported service.
func (m *Manager) DeleteImport(ctx context.Context, name types.NamespacedName) error {
m.logger.Infof("Deleting import '%s/%s'.", name.Namespace, name.Name)

// delete user service
errs := make([]error, 3)
errs := make([]error, 4)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I realize it is not part of the new code, but I wonder if using append won't be a cleaner solution instead of assigning specific array entries. Unless, of course, there is a reliance on the specific order even when there are nil errors in the array.
Alternately, this might be better served by errors.Join().
@kfirtoledo @orozery - not familiar with this code well enough to say. Please weigh in.

pkg/operator/controller/instance_controller.go Outdated Show resolved Hide resolved
}
}
if data, ok := cm.Data["Corefile"]; ok {
// remove trailing end-of-line
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you use the file / hosts plugins there might already be packages for parsing their data (I imagine the pluiin needs it anyway)

pkg/controlplane/control/manager.go Outdated Show resolved Hide resolved
pkg/controlplane/control/manager.go Outdated Show resolved Hide resolved
pkg/controlplane/control/manager.go Outdated Show resolved Hide resolved
return err
}
}
if data, ok := cm.Data["Corefile"]; ok {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may want to move rewrite parsing into its own package/struct so that the file parsing is not split across these functions as internal implementation

@elevran elevran changed the title Add CoreDns rewrite support for external services. Add CoreDNS rewrite support for external services. Jun 19, 2024
aviweit and others added 2 commits June 19, 2024 14:51
Co-authored-by: Etai Lev Ran <[email protected]>
Signed-off-by: Avi Weit <[email protected]>
Co-authored-by: Etai Lev Ran <[email protected]>
Signed-off-by: Avi Weit <[email protected]>
pkg/controlplane/control/dns.go Outdated Show resolved Hide resolved
pkg/controlplane/control/dns.go Outdated Show resolved Hide resolved
pkg/controlplane/control/manager.go Outdated Show resolved Hide resolved
@orozery
Copy link
Collaborator

orozery commented Jun 23, 2024

@aviweit can you give examples of aliases (those you set in Import.spec.alias) for your use-case?

@aviweit
Copy link
Author

aviweit commented Jun 23, 2024

@aviweit can you give examples of aliases (those you set in Import.spec.alias) for your use-case?

@orozery ,

  • vpce-03e2c268057b83e8b-o9f5atr7.lambda.us-east-1.vpce.amazonaws.com
  • *.vpce-0e1b0638f39353e77-ls3zxnij.s3.us-east-1.vpce.amazonaws.com

@aviweit
Copy link
Author

aviweit commented Jun 24, 2024

The PR is set with the discussed changes.

It was tested with kind and AWS EKS clusters.

Thanks.

@aviweit
Copy link
Author

aviweit commented Jun 25, 2024

I have tested this PR with two kind clusters using various Import CRs some of them with invalid alias:

  1. during the rollout (patch) there is always at least one coredns pod in running state; I am able to curl www.google.com
  2. if the coredns pods fail to start, there is still at least one pod in running state (probably being configured with the old version of config map); if additional Imports are being added, configmap is updated, however, the running coredns pod still uses the old good version
  3. when deleting the corrupted Import CR(s), rollout is success and coredns pods are available and the other Imports that are valid, can be served.
echo "
apiVersion: clusterlink.net/v1alpha1
kind: Import
metadata:
  name: nginx-project
  namespace: default
spec:
  port:  80
  alias: www.example.com
  sources:
    - exportName:       nginx-project
      exportNamespace:  default
      peer:             cl-peer1
" | kubectl --kubeconfig ~/.kube/cl-peer2 apply -f -
echo "
apiVersion: clusterlink.net/v1alpha1
kind: Import
metadata:
  name: nginx-project-wildcard
  namespace: default
spec:
  port:  80
  alias: \"*.wildcard.example.com\"
  sources:
    - exportName:       nginx-project
      exportNamespace:  default
      peer:             cl-peer1
" | kubectl --kubeconfig ~/.kube/cl-peer2 apply -f -
echo "
apiVersion: clusterlink.net/v1alpha1
kind: Import
metadata:
  name: nginx-project-another
  namespace: default
spec:
  port:  80
  alias: www.example-another.com
  sources:
    - exportName:       nginx-project
      exportNamespace:  default
      peer:             cl-peer1
" | kubectl --kubeconfig ~/.kube/cl-peer2 apply -f -
echo "
apiVersion: clusterlink.net/v1alpha1
kind: Import
metadata:
  name: nginx-project-no-alias
  namespace: default
spec:
  port:  80
  sources:
    - exportName:       nginx-project
      exportNamespace:  default
      peer:             cl-peer1
" | kubectl --kubeconfig ~/.kube/cl-peer2 apply -f -

echo "
apiVersion: clusterlink.net/v1alpha1
kind: Import
metadata:
  name: nginx-project-invalid
  namespace: default
spec:
  port:  80
  alias:  $www.example.com
  sources:
    - exportName:       nginx-project
      exportNamespace:  default
      peer:             cl-peer1
" | kubectl --kubeconfig ~/.kube/cl-peer2 apply -f -
echo "
apiVersion: clusterlink.net/v1alpha1
kind: Import
metadata:
  name: nginx-project-invalid-2
  namespace: default
spec:
  port:  80
  alias:  \"*.www....example-another.com\"
  sources:
    - exportName:       nginx-project
      exportNamespace:  default
      peer:             cl-peer1
" | kubectl --kubeconfig ~/.kube/cl-peer2 apply -f -
echo "
apiVersion: clusterlink.net/v1alpha1
kind: Import
metadata:
  name: nginx-project-invalid-3
  namespace: default
spec:
  port:  80
  alias:  www www.example-another.com
  sources:
    - exportName:       nginx-project
      exportNamespace:  default
      peer:             cl-peer1
" | kubectl --kubeconfig ~/.kube/cl-peer2 apply -f -

I have also tested with two AWS EKS clusters in respect to internal service (e.g. incluster nginx-project) and external ones e.g. AWS s3, AWS lambda.

The policies being used for all tests where "allow all".

config/operator/rbac/role.yaml Show resolved Hide resolved
config/operator/rbac/role.yaml Show resolved Hide resolved
return err
}
}
if data, ok := cm.Data["Corefile"]; ok {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a CoreDNS package to parse and edit Corefile instead of manually manipulating string?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signed-off-by: Avi Weit <[email protected]>
@kfirtoledo kfirtoledo requested a review from elevran June 25, 2024 09:18
aviweit added 5 commits June 25, 2024 11:10
Signed-off-by: Avi Weit <[email protected]>
Signed-off-by: Avi Weit <[email protected]>
Signed-off-by: Avi Weit <[email protected]>
@elevran
Copy link
Collaborator

elevran commented Jun 26, 2024

@aviweit - thank you for this contribution - we're looking for the the best way to make it available in ClusterLink, safely. Thus, I'm marking it as on hold for now.

The main concern is that Imports are namespace scoped but using an alias has cluster-wide impact. This would elevate a user from having only namespace local permissions to affecting workloads across the entire cluster (e.g., the DNS alias could be set to anything, including grabbing service names from another namespace or affecting external services used by workloads in another namespace).

The maintainers will follow up on this work to address the above concern and allow users to experiment with it.
For example, we're considering putting this behind a feature flag (with default set to disabled), adding policies for which aliases are allowed in which namespace, using pod.Spec.hostAliases to affects specific Pods in only the namespace where the import is defined, etc.

Copy link
Collaborator

@elevran elevran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orozery @kfirtoledo need to figure out how to enable this functionality while giving administrators control over what can be aliased without negatively affecting entire cluster or "escaping" from the import's namespace

@elevran
Copy link
Collaborator

elevran commented Jul 11, 2024

@aviweit - thank you for the feature enhancement proposal and PR work that accompanied it.

At this point, we feel that using Import aliases (a namespace scoped construct) to affect CoreDNS configuration (a cluster wide resource, typically under tight administrative control) would not be something we can maintain securely over the long run. Thus, I will close the current PR.

We are exploring alternative ways to support this. One possible approach might be adding a per namespace DNS server to provide aliases (which could be based on CoreDNS and the rewrites approach you implemented). This would constrain the effects to the namespace where import aliases are defined and is under full control of namespace users to opt-in (install DNS service and configure workloads to use it).

Thank you again for the PR!

@elevran elevran closed this Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve DNS support
4 participants